-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is starting to look pretty good, thanks!
I have a few comments that I think would be good discussing / addressing.
Also, I think which would be awesome to have would be to add some tests in |
@fmassa I think it should be consistent with Detectron now. |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
I'll have another closer look today, but would you mind adding some tests in |
@fmassa I'm not familiar with how to write the tests. |
Ok, no worries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few more comments, but it's not a full review yet.
@fmassa I've added some tests about segmentation_mask. However, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need all those files for this test.
Also, can you give me what is the difference between both implementations (the numerical difference) so that I understand a bit better what's the problem?
For |
For the Also, visualizing both results in the image space is going to be very helpful. |
@fmassa For |
Ok, this is good progress, thanks! I'd need to look more closely to see where the difference might come from for the For |
If I use the |
@JoyHuYY1412 you'd need to check the interpolation (to use bilinear instead of nearest), apart from that, the rest should be unchanged (or almost). @txytju do you mind finishing this PR, given that you managed to make it work for your case? |
|
I like this! |
I faced a problem with
This happened because I called |
@IssamLaradji I think it should be And I guess |
@botcs if you could write unit tests for this PR, it would be awesome! |
using the available |
@botcs that's awesome, thanks for the notebook! One last thing I'd like to do to verify if this boundary effects actually matter is to run a training with https://github.com/facebookresearch/maskrcnn-benchmark/blob/master/configs/quick_schedules/e2e_mask_rcnn_R_50_FPN_quick.yaml , which is a fast training and should take ~10 min to run, using the For that, I'd (locally, this is not to be commited) modify maskrcnn-benchmark/maskrcnn_benchmark/data/datasets/coco.py Lines 82 to 84 in f8b0118
so that it uses Mask instead.
I'd expect to have as results something like
, so box AP should be around 8.2, and mask AP around 7.5 Could you do that? Thanks! |
s += "image_height={}, ".format(self.size[1]) | ||
s += "mode={})".format(self.mode) | ||
return s | ||
|
||
|
||
|
||
class Polygons(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the mode
field for the Polygon is completely irrelevant.
It is never used, but causes:
- additional argument passing when constructing
- a headache when trying to find out what Polygon really is
Question:
Wouldn't it be more consistent if the convert
method of a Polygon would be renamed to convert_to_mask
and would return a Mask instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I agree with your points.
The reason why this is currently the case was that I wanted to keep the same interface between Polygons
and Box
(which is not implemented, but is the single-box equivalent of BoxList
).
And my original idea was that we would be able to specify what was the underlying type of the data via the mode: is it a polygon, or a mask?
I'm not sure about changing the convert
name of the method though.
In general, I think both box_list
and segmentation_mask
could benefit from some better design / cleanup, but I'm not sure what that would be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And my original idea was that we would be able to specify what was the underlying type of the data via the mode: is it a polygon, or a mask?
I think I cannot follow this part:
To specify what was the underlying type of the data via the mode
As in the current implementation, a Polygon
instance:
- can be initialized either with a
list
of polygons - can be initialized either with a
Polygon
instance (which is referenced now, but should be hard-copied IMO) - cannot be initialized with a
Mask
, which feature could be added if necessary (I am doing this to convert GTA binary masks to COCO Polygon format, but only because the binary masks are not supported).
So the underlying data would be specified: Polygon.
On the other hand, about the convert
function:
I'm not sure about changing the
convert
name of the method though.
- The
convert
function takes an argument for the target mode, but actually it accepts just a single answer, which is odd. - If I assume that a
Polygon
can be onlyconvert
-ed to aMask
thanconvert
name is OK, but relies on the assumption that the data can be represented either inPolygon
s orMask
s and nothing else, which is not necessary a trivial assumption, so changing the name toconvert_to_mask
would be clear from the very first encounter.
keep the same interface
- We should in this case add the
convert
orconvert_to_polygon
method to theMask
class as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are all reasonable points, and I'm willing to accept PRs that improve the overall consistency and software design of the codebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have trained the model, which went fine, but the evaluation has failed with the following error:
the whole output can be foun at this gist |
I have suppressed the error by using single empty instance with an all-zero mask when the provided It is quite below the expected performance, and I now rerun the training to see if it is still the case. |
So I was curious about the expected results in @fmassa 's comment if they were correct, and I have ran the training using
Which means that the |
In the code, should be round(float(b)) ? |
Hi guys, A few days ago @fmassa mentioned in one of his comments the following:
So I tried to reshape things a bit to accommodate better the different requirements, but it has radically changed a few concepts. I would be keen to learn about your opinions on it: #473 |
I am getting this with this code,
|
@IssamLaradji does this also happen with #473 ? |
@IssamLaradji I think it should be |
Does this work yet for RLE? I am getting this when I pass a list of RLEs.
|
AttributeError: 'list' object has no attribute 'shape' |
Hi @ShihuaiXu , |
No description provided.